Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix #418: addCommandHandler: Returning False for Existing Built-in Commands #3017

Merged
merged 1 commit into from
May 21, 2023
Merged

Fix #418: addCommandHandler: Returning False for Existing Built-in Commands #3017

merged 1 commit into from
May 21, 2023

Conversation

CrosRoad95
Copy link
Contributor

@CrosRoad95 CrosRoad95 commented May 19, 2023 •
edited by patrikjuvonen
Loading

Fixes #418

iprint(addCommandHandler("help", function()end)) -- false
iprint(addCommandHandler("asd", function()end)) -- true
  • fix tiny bug where server console commands are added after resource starts what would make "help" command in example above to return true for first time

@lopezloo lopezloo added the bug Something isn't working label May 20, 2023
@patrikjuvonen patrikjuvonen changed the title Fix 418, addCommandHandler: Returning False for Existing Built-in Commands Fix #418, addCommandHandler: Returning False for Existing Built-in Commands May 21, 2023
patrikjuvonen
patrikjuvonen approved these changes May 21, 2023
View reviewed changes
Copy link
Contributor

@patrikjuvonen patrikjuvonen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, seems fine to me.

@patrikjuvonen patrikjuvonen merged commit 5947012 into multitheftauto:master May 21, 2023
@patrikjuvonen patrikjuvonen added this to the 1.6.0 milestone May 21, 2023
@patrikjuvonen patrikjuvonen changed the title Fix #418, addCommandHandler: Returning False for Existing Built-in Commands Fix #418: addCommandHandler: Returning False for Existing Built-in Commands May 21, 2023
@samr46
Copy link
Contributor

samr46 commented May 23, 2023

This broke "radar" bind. No longer possible to replace default Map using this command.

Previously it was possible if "radar" control is disabled. And it's useful.

toggleControl("radar", false)
addCommandHandler("radar", function(cmd) outputChatBox(cmd) end)

@CrosRoad95
Copy link
Contributor Author

#3031 (comment) - maybe it should be just backwards incompatible change

@samr46
Copy link
Contributor

samr46 commented May 23, 2023 •
edited
Loading

maybe it should be just backwards incompatible change

What's the point of removing simple and clean way of using "radar" command to replace Map? The alternative would be tracking user binds or binding custom map regardless of user selected binds. Unnecessary additional logic.

I'm also wondering what was the point of fixing this original minor issue in a way that breaks this. I think if you really want to fix it then you have to do it right and not simply block all default MTA commands. Because in fact it was always possible to use some of default commands (the ones that share control / default MTA binds) in addCommandHandler.

patrikjuvonen added a commit that referenced this pull request Jun 12, 2023
@patrikjuvonen
Revert (Controversial) "Fix #418: addCommandHandler returns true when…
ee9e71a 
… it should return false in certain scenarios (#3017)"

Requires more input and conversation.

This reverts commit 5947012.
@patrikjuvonen
Copy link
Contributor

People are angry about the change, hence I've reverted it now.

If you want this implemented in the future, please gather more input first and make sure everyone is happy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patrikjuvonen patrikjuvonen patrikjuvonen approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
1.6.0
Development

Successfully merging this pull request may close these issues.

AddCommandHandler problem
4 participants
@CrosRoad95 @samr46 @patrikjuvonen @lopezloo